You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This creates a loop where we run the TemplateSimplifier -> varids/links/AST/SymbolDatabase/ValueTypes, so on the second round of TemplateSimplifier we can use type information from the SymbolDatabase. Not all the passes need to run with TemplateSimplifer so there is a SymbolDatabase::finalize().
On a naive run, it would delete all the AST/SymbolDatabase information, but this can be kind of slow. So I added some incremental updates by tracking where new tokens are added(plus the function bodies around changed call sites), and then refactoring functions to work on token ranges, so there is now TokenList::createAst(start, end) and in SymbolDatabase: addSymbolsForNewTokenRanges(), updateFunctionAndVariablePointers(), a range-limited setValueTypeInTokenList() and findAllScopes(const Token* startToken, const Token* endToken, Scope* startScope). Also the createSymbolDatabaseFindAllScopes internals were split into reusable per-scope/per-function helpers to support this.
Now this only does incremental updates on function instantiation because its much simpler to do as it only adds a Function, its scope and locals(and the only stale references in old code are the renamed call sites, which updateFunctionAndVariablePointers() re-resolves by name). Instantiating a class introduces new functions and types that could be resolved in other parts of the code. So for this case it does a full rebuild.
Also the template alias simplifications requires a full build as well as it restructures existing tokens throughout the list, not just in new ranges. So the "unchanged tokens keep valid info" premise of the incremental update no longer holds.
We can probably address these in the future but will require a larger refactor. I also added a --template-full-rebuild so we can debug any issue with the incremental updates.
So I ran some numbers comparing the time with valueflow disabled:
file
branch
main
delta
loop time
sdb+finalize (br vs main)
valueflow.cpp
0.584s
0.456s
+0.128s (+28%)
0.121s
0.094s vs 0.083s
tokenize.cpp
0.611s
0.525s
+0.086s (+16%)
0.088s
0.092s vs 0.096s
symboldatabase.cpp
0.478s
0.402s
+0.075s (+19%)
0.101s
0.072s vs 0.077s
checkclass.cpp
0.316s
0.239s
+0.077s (+32%)
0.059s
0.044s vs 0.043s
checkother.cpp
0.291s
0.215s
+0.076s (+35%)
0.055s
0.041s vs 0.039s
astutils.cpp
0.263s
0.207s
+0.055s (+27%)
0.050s
0.037s vs 0.033s
templatesimplifier.cpp
0.249s
0.197s
+0.052s (+27%)
0.034s
0.035s vs 0.035s
checkstl.cpp
0.228s
0.187s
+0.041s (+22%)
0.044s
0.031s vs 0.030s
vf_analyzers.cpp
0.208s
0.147s
+0.061s (+41%)
0.057s
0.019s vs 0.021s
programmemory.cpp
0.194s
0.158s
+0.037s (+23%)
0.036s
0.023s vs 0.025s
It seems to be about ~20-30% slower, which is much better than what it was before at ~80% slower doing full rebuilds. Partial rebuilds seem to really help a lot with this.
Obviously since we are doing more instantiations than before it is going to be slower even as there may be room to improve this further. I think this is an acceptable slowdown and in the end I think this will still be faster than the approach in #8688 as we will have to always compute the valuetypes twice since they are seperate components. So I think this approach is ultimately better.
@danmar@chrchr-github What are you thoughts on this approach? I can work on cleaning up this PR and fixing the CI failures if you think this is the better way to go.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This creates a loop where we run the TemplateSimplifier -> varids/links/AST/SymbolDatabase/ValueTypes, so on the second round of
TemplateSimplifierwe can use type information from theSymbolDatabase. Not all the passes need to run withTemplateSimpliferso there is aSymbolDatabase::finalize().On a naive run, it would delete all the AST/SymbolDatabase information, but this can be kind of slow. So I added some incremental updates by tracking where new tokens are added(plus the function bodies around changed call sites), and then refactoring functions to work on token ranges, so there is now
TokenList::createAst(start, end)and in SymbolDatabase:addSymbolsForNewTokenRanges(),updateFunctionAndVariablePointers(), a range-limitedsetValueTypeInTokenList()andfindAllScopes(const Token* startToken, const Token* endToken, Scope* startScope). Also thecreateSymbolDatabaseFindAllScopesinternals were split into reusable per-scope/per-function helpers to support this.Now this only does incremental updates on function instantiation because its much simpler to do as it only adds a
Function, its scope and locals(and the only stale references in old code are the renamed call sites, whichupdateFunctionAndVariablePointers()re-resolves by name). Instantiating a class introduces new functions and types that could be resolved in other parts of the code. So for this case it does a full rebuild.Also the template alias simplifications requires a full build as well as it restructures existing tokens throughout the list, not just in new ranges. So the "unchanged tokens keep valid info" premise of the incremental update no longer holds.
We can probably address these in the future but will require a larger refactor. I also added a
--template-full-rebuildso we can debug any issue with the incremental updates.